Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Allow large Parse File uploads #9286

Open
wants to merge 21 commits into
base: alpha
Choose a base branch
from

Conversation

dalyaidan1
Copy link

Pull Request

Issue

Closes: #9283

Approach

During routing if a file size is larger than can be handled by the V8 engine as a string, it is used as a Blob. This changes touches both the FilesRouter and GridFSBucketAdapter where the creation/upload happens, and the file is handled as a Buffer. I also added a new FilesRouter test spec for these changes.

Tasks

  • Add tests
  • Add changes to documentation (guides, repository pages, code comments)

Copy link

Thanks for opening this pull request!


afterAll(async () => {
// clean up the server for resuse
if (server && server.close) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be enough to just call await reconfigureServer() to init the server with the default config?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your right. Missed that, new to testing here.

src/Adapters/Files/FilesAdapter.js Outdated Show resolved Hide resolved
Signed-off-by: Manuel <[email protected]>
Copy link

codecov bot commented Aug 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.74%. Comparing base (1a2da40) to head (f09037f).
Report is 1 commits behind head on alpha.

❗ There is a different number of reports uploaded between BASE (1a2da40) and HEAD (f09037f). Click for more details.

HEAD has 3 uploads less than BASE
Flag BASE (1a2da40) HEAD (f09037f)
11 8
Additional details and impacted files
@@            Coverage Diff             @@
##            alpha    #9286      +/-   ##
==========================================
- Coverage   93.50%   84.74%   -8.76%     
==========================================
  Files         186      186              
  Lines       14807    14831      +24     
==========================================
- Hits        13845    12569    -1276     
- Misses        962     2262    +1300     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mtrezza
Copy link
Member

mtrezza commented Aug 18, 2024

There seems to be some test coverage missing, could you add that?

return reject(err);
});

const iv = crypto.randomBytes(16);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line will be executed in every case, which can be unnecessary. Could you refactor like in the original code?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If its the original order I think I would have to have duplicate initializations for cipher and iv when its a blob case and when else. I could also do the null ternary like I do with cipher, if you would prefer that?


try {
// when working with a Blob, it could be over the max size of a buffer, so we need to stream it
if (typeof Blob !== 'undefined' && data instanceof Blob) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In what circumstances would Blob be undefined?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pulled it from this line in the JS SDK
https://github.com/parse-community/Parse-SDK-JS/blob/3e955cd3944de39a559e38cbac58d86faa892606/src/ParseFile.ts#L119

I think maybe some node versions might not have it?

@mtrezza mtrezza requested a review from a team September 4, 2024 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parse.Files cannot be created if over 512MB
2 participants